Validate package name in create - #119
kunalKumar-13 wants to merge 2 commits into
Conversation
Each dotted part becomes a Python package directory, so a dash or a leading digit produced a package that could not be installed. Fail with a clear message instead, suggesting the underscore form where that would be valid. Only the last path component is checked, since NAME may be a target path. Closes plone#72
There was a problem hiding this comment.
🟡 Changes recommended
The new validation’s error message is inaccurate for keyword-based failures (and awkward for multiple invalid parts), which is user-facing and should be corrected before release.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds upfront validation for plonecli create package names to prevent generating projects whose dotted components can’t become importable Python package directories (e.g., components containing -, starting with digits, or being Python keywords), addressing issue #72.
Changes:
- Add
_validate_package_name()and invoke it at the start of thecreatecommand. - Provide a targeted underscore suggestion only when it results in a valid dotted name.
- Add a dedicated test module covering valid/invalid names, path inputs, and suggestion behavior; update
CHANGES.md.
File summaries
| File | Description |
|---|---|
plonecli/cli.py |
Introduces and wires in package-name validation for create, including improved CLI error feedback. |
tests/test_package_name_validation.py |
Adds unit tests for name/path validation and messaging behavior. |
CHANGES.md |
Documents the behavior change for the upcoming release. |
Review details
Suppressed comments (1)
plonecli/cli.py:118
- The error message says the offending parts "is not a valid Python identifier", but
badalso includes reserved keywords (e.g.class), which are valid identifiers. Also, if multiple parts are invalid, the singular wording becomes grammatically incorrect; consider building the message with proper pluralization and explicit keyword mention.
raise click.BadParameter(
f"{name!r} is not a valid package name: "
f"{', '.join(repr(p) for p in bad)} is not a valid Python identifier.{hint}",
param_hint="NAME",
)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
|
|
||
| def _validate_package_name(name): | ||
| """Reject names whose dotted parts are not valid Python identifiers. |
There was a problem hiding this comment.
Good catch, fixed in 2a39588. The message now distinguishes the two cases: when every offending part is a keyword it says 'class' is a Python keyword rather than calling it an invalid identifier, which was misleading because class is a valid identifier and only reserved. The docstring says "a dash, a leading digit or a reserved word" now, and there's a test pinning the wording.
Same commit also fixes something bigger that the scaffolding job was catching: NAME is the output directory, not always a package name. backend_addon asks a package_name question defaulting to dst_path.name, but zope-setup asks project_name, where a hyphen is ordinary — plonecli create zope-setup my-project is in the README and this PR was rejecting it. The validator now checks the value that actually becomes the package name (an explicit -d package_name= when given, the directory otherwise) and only for templates that declare such a question, read from their copier.yml rather than a hardcoded list.
evals/scaffolding --quick went from 21 failures to 0.
NAME is the output directory -- run_create() takes it as target_name and hands it to copier as dst_path. Only some templates turn it into a Python package: backend_addon asks a package_name question defaulting to dst_path.name, and addon is a composite that includes it. zope-setup asks project_name instead, where a hyphen is ordinary and documented (`plonecli create zope-setup my-project`, README). Validating NAME unconditionally therefore rejected legitimate input. It refused that README example, and it failed 21 of the scaffolding evaluations in --quick and 26 in --ci-validation, every one of which scaffolds into a hyphenated workspace directory. Validate the value that will actually become the package name -- an explicit `-d package_name=` when given, the directory name otherwise -- and only for templates that ask for one. Whether a template asks is read from its copier.yml rather than hardcoded, so a template added later is classified by what it declares. A template that cannot be read is treated as not asking: refusing to scaffold because a lookup failed is worse than the bug this guards against. Issue plone#72's case is unaffected -- `create addon collective.new-testcase` is still rejected. Also: say "is a Python keyword" rather than "is not a valid Python identifier" when every offending part is a keyword. `class` IS a valid identifier, it is reserved, and the old wording sent the reader looking for a typo that was not there. evals --quick: 21 failures before, 0 after. 235 tests pass, ruff clean.
|
Disclosure I owe this PR: #79 by @arky is also open against #72 and I did not spot it when I opened this. It predates this by four years (May 2022). The two are not the same change, which is why I am flagging rather than closing:
Documentation and enforcement, not two attempts at one fix — and they compose fine, since the README note explains the rule that this now enforces. I have no claim to precedence here. If maintainers prefer the documentation-only route, close this and take #79; it has been waiting a long time. If you would rather enforce it, this is ready, and #79's wording would still be a good addition on top. @arky apologies for arriving on top of your PR without acknowledging it. |
|
Correcting myself: there is a third PR against #72 that I missed, and it matters more than #79. @arky also opened #77, "Check project name for special chars", on the same day in May 2022 — the same validation approach as this PR. He closed it himself within the day, with no maintainer comment, calling it "a quick and dirty fix", and opened the README note in #79 instead. Worth knowing why it did not work, because it is the reason validation looked like a dead end in 2022. #77 rejected any name containing punctuation: if set(name).intersection(set(string.punctuation)):
raise NameError('Name contains special characters')A dot is punctuation, and dotted names are the Plone convention. Running both rules over real names:
So #77 rejected essentially every valid Plone package name while still letting This PR splits on dots and asks whether each part is a Python identifier and not a keyword, which is the actual rule — each part becomes an importable directory. That admits the dotted convention, still catches the None of this is a criticism of @arky — he found the bug, filed it, and tried the fix four years before I did. It is context for choosing between the three: #79 documents the rule, this enforces it, and #77 shows the shape of enforcement to avoid. |
Closes #72.
plonecli create addon collective.new-testcasegenerated a package that could not be installed — each dotted part becomes a Python package directory, so a dash produced an invalid module name and buildout failed later with an opaqueEntryPointerror.createnow rejects the name up front:The underscore suggestion only appears when it would actually produce a valid name, so
collective.2foogets the error without a misleading hint.Went with rejecting rather than normalising, per the issue title — silently rewriting the name the user typed seemed worse than telling them. Happy to switch to normalisation if you'd rather have that.
One thing worth flagging:
NAMEcan be a target path, not just a bare name — the existing tests pass things like/tmp/.../my.addon. Validation therefore only checks the last path component. I only found this because the existing suite caught it.14 tests added. Full suite: 230 passed, 16 skipped.
ruff checkandruff format --checkclean on both files.